Skip to content

Conversation

@stwiname
Copy link

@stwiname stwiname commented Oct 2, 2025

Implements a Delegation Pool contract

Overview

A Delegation Pool contract that allows aggregation of SQT to be pooled together and used to delegate to indexers.
Delegation should behave the same as if the user had delegated directly to an indexer. There is a manager (owner) of the pool
that determines the delegation strategy to indexers.

Details

Delegation

Users delegate their SQT to the pool and receive pool shares (ERC20 tokens) representing their stake in the pool. The pool manager can then
allocate those funds to the indexers they choose to optimise rewards.

Undelegation

Users can then trigger undelegation which will burn their shares and
start a withdrawal process that then allows the user to withdraw their SQT after the lock period which is the same duration as if they had
directly delegated.

Withdrawls

After the lock period, users can withdraw their SQT and rewards from the pool, minus and pool and unbonding fees.

Managers

The pool manager is responsible for making sure the available assets in the pool are delegated to indexers to earn rewards.
The manager can do all the same functionality as delegating directly to an indexer such as delegate, undelegate, redelegate and claim rewards.
The manager can also trigger auto-compounding of rewards which claims rewards from all indexers as well as set a reward fee.

Summary by CodeRabbit

  • New Features

    • Added a pooled delegation contract so users can delegate SQT into a shared pool, receive pool shares, earn proportional rewards, and perform withdrawals.
    • Built-in fee collection and auto-compounding for rewards; view helpers for balances, share price, pending rewards, and pending unbonds.
  • Documentation

    • Added a comprehensive developer guide covering project overview, workflows, deployment, architecture, and upgrade safety.
  • Tests

    • Added extensive integration and unit tests validating delegation, undelegation, withdrawals, fees, compounding, and manager operations.
  • Scripts / Tooling

    • New deployment and upgrade scripts for the delegation pool and an npm script to run them.
  • Chores

    • Added new error codes/messages for delegation-related validations.

@ianhe8x ianhe8x requested review from Copilot and ianhe8x October 10, 2025 04:38
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR implements a comprehensive Delegation Pool contract that allows users to pool their SQT tokens for delegation to indexers, with automated reward management and fee collection. The pool manager can allocate funds to indexers to optimize rewards while users receive ERC20 shares representing their stake.

Key changes:

  • Adds a new DelegationPool contract with delegation, undelegation, and withdrawal functionality
  • Implements era-based share pricing to prevent reward manipulation attacks
  • Provides comprehensive test coverage with 1,340 test lines and mock contracts

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
contracts/DelegationPool.sol Main delegation pool contract implementing ERC20 shares, manager functions, and auto-compounding
test/DelegationPool.test.ts Comprehensive test suite covering all functionality with edge cases and integration tests
contracts/mocks/MockStakingManager.sol Mock contract for testing auto-compound functionality with simulated rewards
contracts/interfaces/IStakingManager.sol Extended interface with delegation and reward management functions
publish/revertcode.json Added error codes (DP001-DP015) for delegation pool specific errors
contracts/RewardsPool.sol Minor type consistency fixes (uint to uint256)
contracts/RewardsBooster.sol Minor type consistency fix (uint to uint256)
CLAUDE.md Added development documentation for AI assistance

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@ianhe8x
Copy link
Contributor

ianhe8x commented Oct 10, 2025

@codex review

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

Comment on lines 485 to 520
/**
* @dev Automatic compound rewards for all active delegations
*/
function autoCompound() external {
require(activeIndexers.length > 0, 'DP013');

// Update share price for new era BEFORE claiming rewards
// This ensures any new deposits use the old price (no reward benefit)
_updateSharePriceIfNeeded();

IStakingManager stakingManager = IStakingManager(
settings.getContractAddress(SQContracts.StakingManager)
);
IERC20 sqToken = IERC20(settings.getContractAddress(SQContracts.SQToken));

uint256 rewardsBefore = sqToken.balanceOf(address(this));
stakingManager.batchStakeReward(activeIndexers);
uint256 rewardsAfter = sqToken.balanceOf(address(this));

uint256 totalRewards = rewardsAfter - rewardsBefore;

if (totalRewards == 0) {
return; // No rewards to compound
}

// Calculate and deduct fees from rewards
uint256 feeAmount = 0;
if (feePerMill > 0) {
feeAmount = (totalRewards * feePerMill) / PER_MILL;
accumulatedFees += feeAmount;
emit FeesDeducted(totalRewards, feeAmount);
}

// Add remaining rewards to available assets for compounding
uint256 compoundAmount = totalRewards - feeAmount;
availableAssets += compoundAmount;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Recompute share price after autoCompound adds rewards

In autoCompound the contract locks the era share price via _updateSharePriceIfNeeded() before calling batchStakeReward and never updates it again once availableAssets increases (lines 488‑520). Deposits executed later in the same era continue minting shares with the stale currentEraSharePrice, so newcomers can buy shares at the pre‑reward rate and capture a portion of the freshly compounded rewards they didn’t contribute. The share price should be recalculated after rewards are added (or rewards should mint new shares) so asset-per-share stays accurate within the era.

Useful? React with 👍 / 👎.

@coderabbitai
Copy link

coderabbitai bot commented Oct 30, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds a new DelegationPool contract and supporting mocks, interfaces, deployment scripts, SDK/type bindings, extensive tests, error codes, and documentation to enable pooled SQT delegation, manager operations, auto-compounding, fee handling, and era-based share pricing.

Changes

Cohort / File(s) Summary
Core Contract
contracts/DelegationPool.sol
New ERC20-compatible pooled delegation contract implementing delegation/undelegation lifecycle, fee management, manager operations (delegate/undelegate/redelegate/cancel), auto-compounding, era-locked share pricing, view helpers, and events.
Interface Extensions
contracts/interfaces/IStakingManager.sol
Added delegation-related methods: delegate, undelegate, redelegate, cancelUnbonding, widthdraw, stakeReward, and batchStakeReward.
Mocks for Testing
contracts/mocks/MockStakingManager.sol
New MockStakingManager implementing IStakingManager for tests: tracks delegations, per-runner rewards, proportional reward distribution, and helper functions to set/fund rewards.
Rewards Code Refinement
contracts/RewardsBooster.sol, contracts/RewardsPool.sol
Changed numeric widths from uint to uint256 for local variables and struct fields (RunnerDeployment), and adjusted loops/indexing to use uint256.
Deployment Scripts
scripts/delegationPool.ts, scripts/deployContracts.ts, scripts/contracts.ts
New deployment/upgrade script for DelegationPool, proxy initialization via ProxyAdmin, added DelegationPool to contracts mapping, getOverrides now accepts wallet, and deploy flows include DelegationPool in outputs.
SDK / Types / Artifacts
src/contracts.ts, src/sdk.ts, src/types.ts
Added DelegationPool artifact export, delegationPool property to ContractSDK, DelegationPool factory to CONTRACT_FACTORY, and new SQContracts enum entry.
NPM Script
package.json
Added deploy:delegation-pool script invoking build and ts-node deployment script.
Error Codes & Docs
publish/revertcode.json, CLAUDE.md
Added DP001–DP015 error codes for DelegationPool validations and updated OPD01 formatting; added CLAUDE.md with developer guidance, architecture, workflows, and upgrade notes.
Tests
test/DelegationPool.test.ts
New comprehensive test suite covering initialization, delegation/undelegation, withdrawals, unbond fee accounting, manager operations, autoCompound flows, era-based pricing, integration with MockStakingManager, and many edge cases.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Pool as DelegationPool
    participant SM as StakingManager
    participant RD as RewardsDistributor

    rect rgb(200,220,255)
    Note over User,Pool: User deposit flow
    User->>Pool: delegate(amount)
    Pool->>Pool: calc shares (era-locked price)
    Pool->>Pool: mint shares, update availableAssets
    end

    rect rgb(220,200,255)
    Note over Pool,SM: Manager delegates pool assets
    Pool->>SM: delegate(runner, amount)
    SM-->>Pool: updated delegation state
    end

    rect rgb(200,255,220)
    Note over Pool,RD: Auto-compound rewards
    Pool->>RD: getPendingRewards(all runners)
    RD-->>Pool: totalRewards
    Pool->>Pool: deduct fees, add net to availableAssets
    end

    rect rgb(255,220,200)
    Note over User,SM: Undelegation and withdrawal
    User->>Pool: undelegate(shares)
    Pool->>Pool: burn shares, compute SQT
    alt availableAssets >= needed
        Pool->>User: transfer SQT (immediate)
    else
        Pool->>SM: undelegate(runner, amount)
        Pool->>Pool: schedule pending unbond, track unbond fees
    end
    User->>Pool: claim withdrawals (after lock)
    Pool->>User: transfer matured SQT
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

  • Areas needing extra attention:
    • contracts/DelegationPool.sol — complex state machine, fee/unbond accounting, era-based price logic, and interactions with multiple external contracts.
    • contracts/mocks/MockStakingManager.sol — reward calculation correctness and parity with expected StakingManager behavior.
    • contracts/RewardsPool.sol and contracts/RewardsBooster.sol — uint→uint256 changes propagating through accounting and loops.
    • Deployment wiring — scripts/deployContracts.ts and scripts/delegationPool.ts interactions with ProxyAdmin and deployment outputs.
    • Tests — test/DelegationPool.test.ts covers many edge cases; verify assumptions and fixtures.

Poem

🐰
I hopped into a pool of code so bright,
Minted shares beneath the moonlit night,
Rewards compounded, fees kept neat,
Runners guided my tiny feet,
A joyful hop — pooled staking takes flight! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main change: introducing a new DelegationPool contract. It directly reflects the primary deliverable and is specific enough for developers scanning history.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch delegation-pool

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4491d22 and 5aac8c9.

📒 Files selected for processing (3)
  • package.json (1 hunks)
  • scripts/contracts.ts (3 hunks)
  • scripts/delegationPool.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • scripts/contracts.ts
  • scripts/delegationPool.ts
  • package.json

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

♻️ Duplicate comments (1)
contracts/DelegationPool.sol (1)

508-546: Share price stays stale after compounding

Line 513 locks the era price before rewards are swept, and nothing refreshes currentEraSharePrice after availableAssets grows. Any deposit later in the same era mints shares at the pre-reward rate and skims part of the compounded rewards from existing holders. You either need to recompute the share price once rewards are credited (without bumping lastPriceUpdateEra) or mint representative shares for the rewards so that asset-per-share remains consistent inside the era.

         // Add remaining rewards to available assets for compounding
         uint256 compoundAmount = totalRewards - feeAmount;
         availableAssets += compoundAmount;
+
+        // Keep the era price in sync with the enlarged asset base
+        if (totalSupply() > 0) {
+            uint256 totalAssets = _calculateTotalAssets();
+            currentEraSharePrice = (totalAssets * 1e18) / totalSupply();
+            totalAssetsAtLastUpdate = totalAssets;
+            totalSharesAtLastUpdate = totalSupply();
+        }
🧹 Nitpick comments (2)
CLAUDE.md (2)

58-66: Specify language for fenced code block.

The fenced code block showing the directory structure should have a language identifier (e.g., text or plaintext) for proper rendering.

Apply this diff:

-```
+```text
 /contracts/
 ├── [Root contracts] - 35+ core contracts (Settings, Staking, IndexerRegistry, etc.)

85-85: Minor grammar improvement.

Consider using a hyphen to join "ethers-rs based" as "ethers-rs-based" for grammatical correctness.

Apply this diff:

-- **Rust SDK**: `/rust/` - ethers-rs based SDK for Rust applications
+- **Rust SDK**: `/rust/` - ethers-rs-based SDK for Rust applications
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 842525f and 4491d22.

📒 Files selected for processing (15)
  • CLAUDE.md (1 hunks)
  • contracts/DelegationPool.sol (1 hunks)
  • contracts/RewardsBooster.sol (1 hunks)
  • contracts/RewardsPool.sol (3 hunks)
  • contracts/interfaces/IStakingManager.sol (1 hunks)
  • contracts/mocks/MockStakingManager.sol (1 hunks)
  • package.json (1 hunks)
  • publish/revertcode.json (1 hunks)
  • scripts/contracts.ts (2 hunks)
  • scripts/delegationPool.ts (1 hunks)
  • scripts/deployContracts.ts (10 hunks)
  • src/contracts.ts (2 hunks)
  • src/sdk.ts (2 hunks)
  • src/types.ts (3 hunks)
  • test/DelegationPool.test.ts (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
scripts/delegationPool.ts (3)
scripts/logger.ts (1)
  • getLogger (20-22)
src/types.ts (2)
  • ContractDeployment (86-89)
  • CONTRACT_FACTORY (107-146)
scripts/deployContracts.ts (1)
  • deployProxy (158-184)
test/DelegationPool.test.ts (2)
test/helper.ts (6)
  • Wallet (40-40)
  • registerRunner (148-163)
  • etherParse (119-122)
  • timeTravel (47-51)
  • constants (36-36)
  • startNewEra (314-319)
scripts/deployContracts.ts (1)
  • deployContracts (343-635)
scripts/deployContracts.ts (1)
test/helper.ts (1)
  • Wallet (40-40)
🪛 LanguageTool
CLAUDE.md

[grammar] ~85-~85: Use a hyphen to join words.
Context: ...SDK - Rust SDK: /rust/ - ethers-rs based SDK for Rust applications - **Cont...

(QB_NEW_EN_HYPHEN)

🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md

58-58: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🔇 Additional comments (7)
contracts/RewardsPool.sol (1)

282-285: Consistent uint256 indexing

Good call aligning the stored counters and loop index to uint256; it keeps the array walk in _batchCollect consistent with the struct storage and avoids implicit casts. Looks solid.

contracts/RewardsBooster.sol (1)

432-432: LGTM: Improved type consistency.

The change from uint to uint256 makes the type explicit and aligns with Solidity best practices for clarity.

package.json (1)

24-24: LGTM: Deployment script properly configured.

The new deployment script follows the existing pattern and conventions used by other deployment scripts in the project.

publish/revertcode.json (1)

225-239: LGTM: Error codes well-defined.

The new DelegationPool error codes (DP001-DP015) are clearly defined with descriptive messages that will help with debugging and user feedback.

src/types.ts (1)

48-48: LGTM: DelegationPool properly integrated into type system.

The additions correctly expose DelegationPool through the factory mapping and enum, following the established patterns for other contracts.

Also applies to: 145-145, 170-170

src/sdk.ts (1)

36-36: LGTM: SDK property correctly added.

The delegationPool property is properly integrated into the ContractSDK class, following the same pattern as other contract properties. The dynamic initialization in _init() will handle the setup automatically.

Also applies to: 79-79

contracts/interfaces/IStakingManager.sol (1)

11-23: All implementations of IStakingManager already provide the required functions.

Verification confirms both implementations are complete:

  • MockStakingManager.sol: all 7 functions present (delegate, undelegate, redelegate, cancelUnbonding, widthdraw, stakeReward, batchStakeReward)
  • StakingManager.sol: all 7 functions present

No compilation errors will occur from missing implementations.

Comment on lines +301 to +316
uint256 feeFromAvailable = MathUtil.mulDiv(
unbondFeeRate,
amountFromAvailable,
PER_MILL
);
uint256 feeFromIndexers = MathUtil.mulDiv(unbondFeeRate, amountFromIndexers, PER_MILL);

// Calculate net amounts after fees
uint256 netFromAvailable = amountFromAvailable - feeFromAvailable;
uint256 netFromIndexers = amountFromIndexers - feeFromIndexers;

// Track fees: accumulated (from available) and expected (from indexers)
accumulatedUnbondFees += feeFromAvailable;
expectedUnbondFees += feeFromIndexers;
emit UnbondFeeTracked(feeFromIndexers, expectedUnbondFees);

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

expectedUnbondFees only ever grows

Lines 313-315 add indexer fees to expectedUnbondFees, but nothing ever subtracts from this accumulator—not when deposits backfill the pending undelegation, not when the manager actually withdraws, and not when users are paid. As a result _calculateTotalAssets() (Line 753) permanently understates assets, depressing the share price and letting new deposits mint extra shares. Please wire in the inverse operation (e.g., track per-request fees and clear them when the corresponding undelegation is cancelled or completed) so the accounting stays balanced.

             // Track fees: accumulated (from available) and expected (from indexers)
             accumulatedUnbondFees += feeFromAvailable;
             expectedUnbondFees += feeFromIndexers;
             emit UnbondFeeTracked(feeFromIndexers, expectedUnbondFees);
+            unbondingRequests[msg.sender][unbondingRequests[msg.sender].length - 1]
+                .expectedFeeFromIndexers = feeFromIndexers;
-        if (pendingUndelegationsForUsers > 0) {
+        if (pendingUndelegationsForUsers > 0) {
             // Use new deposit to cover pending undelegations first
             if (_amount >= pendingUndelegationsForUsers) {
                 // Fully cover pending undelegations
                 availableAssets += (_amount - pendingUndelegationsForUsers);
+                expectedUnbondFees -= _claimableFeeOffset(_amount - (_amount - pendingUndelegationsForUsers));
                 pendingUndelegationsForUsers = 0;
-        stakingManager.undelegate(_runner, _amount);
+        stakingManager.undelegate(_runner, _amount);
+        expectedUnbondFees -= _expectedFeeFor(_runner, _amount);

Committable suggestion skipped: line range outside the PR's diff.


function cancelUnbonding(uint256 unbondReqId) external;

function widthdraw() external;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fix the function name typo.

The function is misspelled as widthdraw() and should be withdraw().

Apply this diff:

-    function widthdraw() external;
+    function withdraw() external;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function widthdraw() external;
function withdraw() external;
🤖 Prompt for AI Agents
In contracts/interfaces/IStakingManager.sol around line 19, the function name is
misspelled as "widthdraw()" — change the declaration to the correct spelling
"withdraw()" so the interface matches expected naming; update any implementing
contracts or references if necessary to use withdraw() instead of widthdraw().

Comment on lines 41 to 45
await deployDelegationPool({
wallet: connectedWallet,
deployment,
});
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Persist the DelegationPool deployment metadata
Lines 41-45 reload the publish JSON, deploy the proxy, and exit without ever updating the deployment file. Downstream scripts continue to see DelegationPool as “not deployed”, so the next run will try to redeploy (or fail when looking up deployment.child.DelegationPool). Please store the new address, logic (innerAddress), and metadata in deployment.child.DelegationPool and call saveDeployment before returning. A sketch:

-import { deployProxy } from './deployContracts';
+import { deployProxy, saveDeployment } from './deployContracts';
+import CONTRACTS from '../src/contracts';
+import sha256 from 'sha256';
@@
-export async function deployDelegationPool({ wallet, deployment }: { wallet: Wallet; deployment: ContractDeployment }) {
+export async function deployDelegationPool({ wallet, deployment }: { wallet: Wallet; deployment: ContractDeployment }) {
@@
-    const [contract, innerAddress] = await deployProxy(proxyAdmin, CONTRACT_FACTORY.DelegationPool, wallet, confirms);
+    const [contract, innerAddress] = await deployProxy(proxyAdmin, CONTRACT_FACTORY.DelegationPool, wallet, confirms);
@@
-    return contract;
+    return { contract, innerAddress };
 }
@@
-    await deployDelegationPool({
+    const { contract, innerAddress } = await deployDelegationPool({
         wallet: connectedWallet,
         deployment,
     });
+
+    deployment.child.DelegationPool = {
+        address: contract.address,
+        innerAddress,
+        bytecodeHash: sha256(Buffer.from(CONTRACTS.DelegationPool.bytecode.slice(2), 'hex')),
+        lastUpdate: new Date().toUTCString(),
+    };
+    saveDeployment(name, deployment);

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In scripts/delegationPool.ts around lines 41 to 45, the deployment function
calls await deployDelegationPool(...) and returns without persisting the new
proxy address, logic (innerAddress), and any returned metadata; update
deployment.child.DelegationPool with the returned values (address,
innerAddress/logic, and metadata) from deployDelegationPool, then call
saveDeployment(deployment) to write the updated deployment file before returning
so downstream scripts see the contract as deployed.

Comment on lines +509 to +582
describe.only('managerUndelegate()', () => {
beforeEach(async () => {
// Setup: Delegate to runner first
await delegationPool.connect(poolManager).managerDelegate(runner1.address, etherParse('2000'));
});

it('should allow manager to undelegate from runner', async () => {
const undelegateAmount = etherParse('1000');

// Make sure all available assets are delegated
await delegationPool
.connect(poolManager)
.managerDelegate(runner2.address, await delegationPool.availableAssets());
// User undelegates to make a pending undelegation
await delegationPool.connect(user1).undelegate(undelegateAmount);

await expect(delegationPool.connect(poolManager).managerUndelegate(runner1.address, undelegateAmount))
.to.emit(delegationPool, 'ManagerUndelegated')
.withArgs(runner1.address, undelegateAmount);

expect(await delegationPool.getDelegatedToIndexer(runner1.address)).to.equal(etherParse('1000'));
});

it('should remove indexer from active list when delegation becomes zero', async () => {
const amount = etherParse('2000');
await delegationPool
.connect(poolManager)
.managerDelegate(runner2.address, await delegationPool.availableAssets());
await delegationPool.connect(user1).undelegate(amount);
await delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount);

expect(await delegationPool.getDelegatedToIndexer(runner1.address)).to.equal(0);
expect(await delegationPool.isActiveIndexer(runner1.address)).to.be.false;
expect(await delegationPool.getActiveIndexersCount()).to.equal(1);
});

it('should reject undelegation exceeding delegated amount', async () => {
const excessiveAmount = etherParse('3000');
await expect(
delegationPool.connect(poolManager).managerUndelegate(runner1.address, excessiveAmount)
).to.be.revertedWith('DP010');
});

it('should reduce the pendingUndelegationsForUsers after manager undelegating', async () => {
const amount = etherParse('3000');
await delegationPool.connect(poolManager).managerDelegate(runner1.address, amount);

// All funds should be delegated
expect(await delegationPool.availableAssets()).to.equal(0);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);

await delegationPool.connect(user1).undelegate(amount);

expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(amount);

await delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);
});

// If manager undelegates before user undelegation, pendingUndelegationsForUsers should not increase
it('not allow manager to undelegate more than users have requested', async () => {
const amount = etherParse('3000');
// Delegate all funds
await delegationPool.connect(poolManager).managerDelegate(runner1.address, amount);

// All funds should be delegated
expect(await delegationPool.availableAssets()).to.equal(0);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);

await expect(
delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount)
).to.be.revertedWith('DP015');
});
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Remove .only to restore the full test suite
Line 509 wraps managerUndelegate() in describe.only, so Mocha will skip every other DelegationPool test. This hides regressions and will make CI report green while most coverage never runs. Please drop the .only.

-        describe.only('managerUndelegate()', () => {
+        describe('managerUndelegate()', () => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
describe.only('managerUndelegate()', () => {
beforeEach(async () => {
// Setup: Delegate to runner first
await delegationPool.connect(poolManager).managerDelegate(runner1.address, etherParse('2000'));
});
it('should allow manager to undelegate from runner', async () => {
const undelegateAmount = etherParse('1000');
// Make sure all available assets are delegated
await delegationPool
.connect(poolManager)
.managerDelegate(runner2.address, await delegationPool.availableAssets());
// User undelegates to make a pending undelegation
await delegationPool.connect(user1).undelegate(undelegateAmount);
await expect(delegationPool.connect(poolManager).managerUndelegate(runner1.address, undelegateAmount))
.to.emit(delegationPool, 'ManagerUndelegated')
.withArgs(runner1.address, undelegateAmount);
expect(await delegationPool.getDelegatedToIndexer(runner1.address)).to.equal(etherParse('1000'));
});
it('should remove indexer from active list when delegation becomes zero', async () => {
const amount = etherParse('2000');
await delegationPool
.connect(poolManager)
.managerDelegate(runner2.address, await delegationPool.availableAssets());
await delegationPool.connect(user1).undelegate(amount);
await delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount);
expect(await delegationPool.getDelegatedToIndexer(runner1.address)).to.equal(0);
expect(await delegationPool.isActiveIndexer(runner1.address)).to.be.false;
expect(await delegationPool.getActiveIndexersCount()).to.equal(1);
});
it('should reject undelegation exceeding delegated amount', async () => {
const excessiveAmount = etherParse('3000');
await expect(
delegationPool.connect(poolManager).managerUndelegate(runner1.address, excessiveAmount)
).to.be.revertedWith('DP010');
});
it('should reduce the pendingUndelegationsForUsers after manager undelegating', async () => {
const amount = etherParse('3000');
await delegationPool.connect(poolManager).managerDelegate(runner1.address, amount);
// All funds should be delegated
expect(await delegationPool.availableAssets()).to.equal(0);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);
await delegationPool.connect(user1).undelegate(amount);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(amount);
await delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);
});
// If manager undelegates before user undelegation, pendingUndelegationsForUsers should not increase
it('not allow manager to undelegate more than users have requested', async () => {
const amount = etherParse('3000');
// Delegate all funds
await delegationPool.connect(poolManager).managerDelegate(runner1.address, amount);
// All funds should be delegated
expect(await delegationPool.availableAssets()).to.equal(0);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);
await expect(
delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount)
).to.be.revertedWith('DP015');
});
});
describe('managerUndelegate()', () => {
beforeEach(async () => {
// Setup: Delegate to runner first
await delegationPool.connect(poolManager).managerDelegate(runner1.address, etherParse('2000'));
});
it('should allow manager to undelegate from runner', async () => {
const undelegateAmount = etherParse('1000');
// Make sure all available assets are delegated
await delegationPool
.connect(poolManager)
.managerDelegate(runner2.address, await delegationPool.availableAssets());
// User undelegates to make a pending undelegation
await delegationPool.connect(user1).undelegate(undelegateAmount);
await expect(delegationPool.connect(poolManager).managerUndelegate(runner1.address, undelegateAmount))
.to.emit(delegationPool, 'ManagerUndelegated')
.withArgs(runner1.address, undelegateAmount);
expect(await delegationPool.getDelegatedToIndexer(runner1.address)).to.equal(etherParse('1000'));
});
it('should remove indexer from active list when delegation becomes zero', async () => {
const amount = etherParse('2000');
await delegationPool
.connect(poolManager)
.managerDelegate(runner2.address, await delegationPool.availableAssets());
await delegationPool.connect(user1).undelegate(amount);
await delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount);
expect(await delegationPool.getDelegatedToIndexer(runner1.address)).to.equal(0);
expect(await delegationPool.isActiveIndexer(runner1.address)).to.be.false;
expect(await delegationPool.getActiveIndexersCount()).to.equal(1);
});
it('should reject undelegation exceeding delegated amount', async () => {
const excessiveAmount = etherParse('3000');
await expect(
delegationPool.connect(poolManager).managerUndelegate(runner1.address, excessiveAmount)
).to.be.revertedWith('DP010');
});
it('should reduce the pendingUndelegationsForUsers after manager undelegating', async () => {
const amount = etherParse('3000');
await delegationPool.connect(poolManager).managerDelegate(runner1.address, amount);
// All funds should be delegated
expect(await delegationPool.availableAssets()).to.equal(0);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);
await delegationPool.connect(user1).undelegate(amount);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(amount);
await delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);
});
// If manager undelegates before user undelegation, pendingUndelegationsForUsers should not increase
it('not allow manager to undelegate more than users have requested', async () => {
const amount = etherParse('3000');
// Delegate all funds
await delegationPool.connect(poolManager).managerDelegate(runner1.address, amount);
// All funds should be delegated
expect(await delegationPool.availableAssets()).to.equal(0);
expect(await delegationPool.pendingUndelegationsForUsers()).to.equal(0);
await expect(
delegationPool.connect(poolManager).managerUndelegate(runner1.address, amount)
).to.be.revertedWith('DP015');
});
});
🤖 Prompt for AI Agents
In test/DelegationPool.test.ts around lines 509 to 582, the top-level describe
for managerUndelegate() uses describe.only which limits Mocha to run only this
block; remove the .only so the test suite runs all tests. Edit the line to use
describe(...) (without .only), save, and re-run the test suite/CI to ensure full
coverage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants